Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change refresh behavior to use --refresh flag #1118

Merged
merged 10 commits into from
Sep 9, 2024

Conversation

fitz-vivodyne
Copy link
Contributor

@fitz-vivodyne fitz-vivodyne commented Mar 10, 2024

As described in #870, the current refresh config option runs pulumi refresh prior to the requested command, updating the state file. There is not currently a way to run pulumi preview --refresh or pulumi up --refresh.

This PR adds a soft-refresh flag which enables the --refresh flag.
Not sure on the right approach for naming, but I figured this helps get across the difference between refresh updating the state and --refresh not updating the state.

After further discussion, this PR represents a breaking change.
As of this PR when refresh = true the existing pulumi refresh && pulumi <command> behavior is removed and commands are instead run with --refresh

Completely untested at this point.
I took a look at existing tests and didn't see an obvious place to add a test.
No local testing yet either, will play with that in a bit, erring on the side of opening early to enable discussion.

@tgummerer
Copy link
Contributor

@fitz-vivodyne Thanks for the PR and sorry about the delay here. I've just let the tests run, and there are some failures. I believe this has the same cause as #1116 (review), and I think the option needs to be added to the action.yml file.

@fitz-vivodyne fitz-vivodyne requested a review from a team as a code owner March 22, 2024 01:56
@fitz-vivodyne
Copy link
Contributor Author

Thanks @tgummerer, just pushed the updates to action.yml

@simenandre
Copy link
Contributor

Hello 👋 Thanks for doing this, @fitz-vivodyne!

I am wondering what you think of keep using the refresh argument, and change the behavior of it to use this?

For most users they expect to be using this way of refreshing. We need to bump major, since it might break things, but it's the same kind of breaking we've done since v2; smaller and easy to migrate.

@fitz-vivodyne
Copy link
Contributor Author

I am wondering what you think of keep using the refresh argument, and change the behavior of it to use this?
For most users they expect to be using this way of refreshing. We need to bump major, since it might break things, but it's the same kind of breaking we've done since v2; smaller and easy to migrate.

@simenandre I found the current behavior surprising and when I first started using the action I 100% expected the refresh argument to just run with the --refresh flag.
If you actually want to run a pulumi refresh I would expect to run that as a standalone action/command.

Agreed that a major version bump would be appropriate, and I'm personally supportive of fully replacing the refresh functionality.

Thoughts on moving forward?
New PR, modify this one to strip out the refresh behavior?

@tgummerer
Copy link
Contributor

I think a new major release and changing the refresh functionality would make sense here as well. I think updating this PR sounds good, but no strong preferences here either way.

@komalali
Copy link
Member

komalali commented Mar 26, 2024

+1 to changing the refresh functionality and bumping the major version. I'm guessing the reason it behaved this way originally was because automation api didn't have support for the refresh flag.

Although, now that I think about it, I think we've only added support for --refresh for Go Automation API so far, so we'll need to port that over to the nodejs automation api to be able to be used here.

EDIT: Looks like we at least have the refresh option for up and preview in nodejs automation api, but would be great to get this supported for destroy as well to have consistency across commands.

@mikhailshilkov
Copy link
Member

@tgummerer I think we should provide the guidance for what we want to happen here. Let's make the decision and try to land this PR, or decide not to land it and describe the alternative.

@tgummerer
Copy link
Contributor

I'd be happy to land this PR if @fitz-vivodyne wants to update it with the updated refresh functionality. Once that's done I can approve, merge and create a new release with a major version bump.

@mikhailshilkov
Copy link
Member

@fitz-vivodyne Will you have time to take another step on this PR?

@fitz-vivodyne
Copy link
Contributor Author

Sorry, haven't had bandwidth recently.
Might be a while before I'm able to get around to it, anyone else can feel free to grab it if they're available

@mikhailshilkov
Copy link
Member

@fitz-vivodyne Do you mind converting it to a Draft PR for now, so that it doesn't show up in our review queue?

@fitz-vivodyne fitz-vivodyne marked this pull request as draft May 10, 2024 17:23
@fitz-vivodyne fitz-vivodyne marked this pull request as ready for review September 4, 2024 21:54
@fitz-vivodyne
Copy link
Contributor Author

Alright, I find myself wanting this functionality again so I just pushed the discussed changes to the PR.

This now represents a breaking change/major release that would change the behavior of refresh: true to run commands with --refresh instead of running pulumi refresh && pulumi <command>

@fitz-vivodyne fitz-vivodyne changed the title Add soft-refresh config option to enable preview --refresh/up --refresh Change refresh behavior to use --refresh flag Sep 4, 2024
Copy link
Contributor

@tgummerer tgummerer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat, thank you!

@tgummerer tgummerer merged commit f1948a3 into pulumi:main Sep 9, 2024
76 checks passed
tgummerer added a commit that referenced this pull request Sep 25, 2024
This includes pulumi/pulumi#17209, which we
need to fully support the changes made in
#1118, and thus the 6.0.0
release of the pulumi action.
tgummerer added a commit that referenced this pull request Sep 26, 2024
This includes pulumi/pulumi#17209, which we
need to fully support the changes made in
#1118, and thus the 6.0.0
release of the pulumi action.
@pulumi-bot
Copy link

This PR has been shipped in release v6.0.0.

@mikocot
Copy link
Contributor

mikocot commented Oct 1, 2024

@fitz-vivodyne @tgummerer as long as the effect of this change on pulumi preview has been explained and is clear to me' what is the change to pulumi up command? Is that just for consistency but we expect the same effect, with precison to maybe error-handling, or there is a significant difference in behavior too?

I believe this info should be included in the 'Migration from v5 section of the readme'. I think most people care about the actual behavior change than what is updated in the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants